From 33c0321394bc787edc82a5ef21fdbefa2ae857bc Mon Sep 17 00:00:00 2001 From: tsteven4 <13596209+tsteven4@users.noreply.github.com> Date: Mon, 27 Jan 2020 19:08:51 -0700 Subject: [PATCH] rework gathering of serialization information. (#479) The existing method broke encapsulation to generate the combined list of regular and style based formats. It required access to xcsv_vecs, which is only present in LegacyFormats. Until recently it also required access to xcsv_file. The existing method also leaked memory, although the use case was such that the program always exited shortly thereafter. All these issues are resolved during unification by gathering all the necessary information and passing that on to serialization. This information is gathered from different places for regular and style based formats. --- testo.d/serialization.test | 64 +++++++------- vecs.cc | 176 +++++++++++++++++++------------------ vecs.h | 37 +++++++- 3 files changed, 155 insertions(+), 122 deletions(-) diff --git a/testo.d/serialization.test b/testo.d/serialization.test index 9423a8d74..491bb94b0 100755 --- a/testo.d/serialization.test +++ b/testo.d/serialization.test @@ -3,38 +3,34 @@ # These outputs will change with any new filter or changed filter option. # Filter options must be hand coded in the GUI. # -# we have memory leaks in sort_and_unify. -if [ ${RUNNINGVALGRIND} -ne 0 ]; then - # These are primarily meant to serialize the interface specification to - # the GUI and the document. - # We do a compare_nole as specific whitespace is part of deserialization. - gpsbabel -^3 > ${TMPDIR}/format3.txt - compare_nole ${REFERENCE}/format3.txt ${TMPDIR}/format3.txt - gpsbabel -^2 > ${TMPDIR}/format2.txt - compare_nole ${REFERENCE}/format2.txt ${TMPDIR}/format2.txt - gpsbabel -^1 > ${TMPDIR}/format1.txt - compare_nole ${REFERENCE}/format1.txt ${TMPDIR}/format1.txt - gpsbabel -^0 > ${TMPDIR}/format0.txt - compare_nole ${REFERENCE}/format0.txt ${TMPDIR}/format0.txt - gpsbabel -%1 > ${TMPDIR}/filter1.txt - compare_nole ${REFERENCE}/filter1.txt ${TMPDIR}/filter1.txt - gpsbabel -%0 > ${TMPDIR}/filter0.txt - compare_nole ${REFERENCE}/filter0.txt ${TMPDIR}/filter0.txt - - # These are primarily meant for a user - gpsbabel -h > ${TMPDIR}/help.txt - # ahh shucks, the executable changes based on OS, and the path can change as well. - sed 's/.*\[options\]/ .\/gpsbabel [options]/' ${TMPDIR}/help.txt > ${TMPDIR}/help.stripped.txt - compare ${REFERENCE}/help.txt ${TMPDIR}/help.stripped.txt - gpsbabel -h gpx > ${TMPDIR}/formatusage.txt - compare ${REFERENCE}/formatusage.txt ${TMPDIR}/formatusage.txt - gpsbabel -h radius > ${TMPDIR}/filterusage.txt - compare ${REFERENCE}/filterusage.txt ${TMPDIR}/filterusage.txt - gpsbabel > ${TMPDIR}/usage.txt << EOJ - -EOJ - # ahh shucks, the executable changes based on OS, and the path can change as well. - sed 's/.*\[options\]/ .\/gpsbabel [options]/' ${TMPDIR}/usage.txt > ${TMPDIR}/usage.stripped.txt - compare ${REFERENCE}/usage.txt ${TMPDIR}/usage.stripped.txt -fi +# These are primarily meant to serialize the interface specification to +# the GUI and the document. +# We do a compare_nole as specific whitespace is part of deserialization. +gpsbabel -^3 > ${TMPDIR}/format3.txt +compare_nole ${REFERENCE}/format3.txt ${TMPDIR}/format3.txt +gpsbabel -^2 > ${TMPDIR}/format2.txt +compare_nole ${REFERENCE}/format2.txt ${TMPDIR}/format2.txt +gpsbabel -^1 > ${TMPDIR}/format1.txt +compare_nole ${REFERENCE}/format1.txt ${TMPDIR}/format1.txt +gpsbabel -^0 > ${TMPDIR}/format0.txt +compare_nole ${REFERENCE}/format0.txt ${TMPDIR}/format0.txt +gpsbabel -%1 > ${TMPDIR}/filter1.txt +compare_nole ${REFERENCE}/filter1.txt ${TMPDIR}/filter1.txt +gpsbabel -%0 > ${TMPDIR}/filter0.txt +compare_nole ${REFERENCE}/filter0.txt ${TMPDIR}/filter0.txt + +# These are primarily meant for a user +gpsbabel -h > ${TMPDIR}/help.txt +# ahh shucks, the executable changes based on OS, and the path can change as well. +sed 's/.*\[options\]/ .\/gpsbabel [options]/' ${TMPDIR}/help.txt > ${TMPDIR}/help.stripped.txt +compare ${REFERENCE}/help.txt ${TMPDIR}/help.stripped.txt +gpsbabel -h gpx > ${TMPDIR}/formatusage.txt +compare ${REFERENCE}/formatusage.txt ${TMPDIR}/formatusage.txt +gpsbabel -h radius > ${TMPDIR}/filterusage.txt +compare ${REFERENCE}/filterusage.txt ${TMPDIR}/filterusage.txt +gpsbabel > ${TMPDIR}/usage.txt << EOJ +EOJ +# ahh shucks, the executable changes based on OS, and the path can change as well. +sed 's/.*\[options\]/ .\/gpsbabel [options]/' ${TMPDIR}/usage.txt > ${TMPDIR}/usage.stripped.txt +compare ${REFERENCE}/usage.txt ${TMPDIR}/usage.stripped.txt diff --git a/vecs.cc b/vecs.cc index 58c40b5ac..500ebb80d 100644 --- a/vecs.cc +++ b/vecs.cc @@ -360,76 +360,88 @@ QString Vecs::get_option(const QStringList& options, const char* argname) } /* - * Smoosh the vecs list and style lists together and sort them - * alphabetically. Returns an allocated copy of a style_vecs_array - * that's populated and sorted. + * Gather information relevant to serialization from the + * vecs and style lists. Sort and return the information. */ -QVector Vecs::sort_and_unify_vecs() const +QVector Vecs::sort_and_unify_vecs() const { - QVector svp; + QVector svp; svp.reserve(vec_list.size() + style_list.size()); - /* Normal vecs are easy; populate the first part of the array. */ + /* Gather relevant information for normal formats. */ for (const auto& vec : vec_list) { - vecs_t uvec = vec; - if (uvec.parent == nullptr) { - uvec.parent = uvec.name; + vecinfo_t info; + info.name = vec.name; + info.desc = vec.desc; + info.extensions = vec.extensions; + if (vec.parent.isEmpty()) { + info.parent = vec.name; + } else { + info.parent = vec.parent; } - svp.append(uvec); + info.type = vec.vec->get_type(); + info.cap = vec.vec->get_cap(); + const QVector* args = vec.vec->get_args(); + if (args != nullptr) { + for (const auto& arg : *args) { + info.arginfo.append(arginfo_t(arg)); + } + } + svp.append(info); } #if CSVFMTS_ENABLED /* The style formats are based on the xcsv format, * Make sure we know which entry in the vector list that is. */ - assert(vec_list.at(0).name == "xcsv"); + assert(vec_list.at(0).name.compare("xcsv", Qt::CaseInsensitive) == 0); /* The style formats use a modified xcsv argument list that doesn't include * the option to set the style file. Make sure we know which entry in * the argument list that is. */ assert(case_ignore_strcmp(vec_list.at(0).vec->get_args()->at(0).helpstring, "Full path to XCSV style file") == 0); - /* Prepare a modified argument list for the style formats. */ - auto xcsv_args = new QVector(*vec_list.at(0).vec->get_args()); /* LEAK */ - xcsv_args->removeFirst(); - /* Walk the style list, parse the entries, dummy up a "normal" vec */ + /* Gather the relevant info for the style based formats. */ for (const auto& svec : style_list) { XcsvStyle style = xcsv_read_internal_style(svec.style_buf); - vecs_t uvec; - uvec.name = svec.name; - uvec.extensions = style.extension; - /* TODO: This needs to be reworked when xcsv isn't a LegacyFormat and - * xcsv_vecs disappear. - */ - auto ffvec = ff_vecs_t(xcsv_vecs); /* Inherits xcsv opts */ - /* Reset file type to inherit ff_type from xcsv. */ - ffvec.type = style.type; - /* Skip over the first help entry for all but the - * actual 'xcsv' format - so we don't expose the - * 'Full path to XCSV style file' argument to any - * GUIs for an internal format. - */ - ffvec.args = xcsv_args; - ffvec.cap.fill(ff_cap_none); + vecinfo_t info; + info.name = svec.name; + info.desc = style.description; + info.extensions = style.extension; + info.parent = "xcsv"; + info.type = style.type; + info.cap.fill(ff_cap_none, 3); switch (style.datatype) { case unknown_gpsdata: case wptdata: - ffvec.cap[ff_cap_rw_wpt] = (ff_cap)(ff_cap_read | ff_cap_write); + info.cap[ff_cap_rw_wpt] = (ff_cap)(ff_cap_read | ff_cap_write); break; case trkdata: - ffvec.cap[ff_cap_rw_trk] = (ff_cap)(ff_cap_read | ff_cap_write); + info.cap[ff_cap_rw_trk] = (ff_cap)(ff_cap_read | ff_cap_write); break; case rtedata: - ffvec.cap[ff_cap_rw_rte] = (ff_cap)(ff_cap_read | ff_cap_write); + info.cap[ff_cap_rw_rte] = (ff_cap)(ff_cap_read | ff_cap_write); break; default: ; } - uvec.vec = new LegacyFormat(ffvec); /* LEAK */ - uvec.desc = style.description; - uvec.parent = "xcsv"; - svp.append(uvec); + /* Skip over the first help entry of the xcsv args. + * We don't want to expose the + * 'Full path to XCSV style file' argument to any + * GUIs for an internal format. + */ + bool first = true; + const QVector* args = vec_list.at(0).vec->get_args(); + if (args != nullptr) { + for (const auto& arg : *args) { + if (!first) { + info.arginfo.append(arginfo_t(arg)); + } + first = false; + } + } + svp.append(info); } #endif // CSVFMTS_ENABLED @@ -437,7 +449,7 @@ QVector Vecs::sort_and_unify_vecs() const * Display the available formats in a format that's easy for humans to * parse for help on available command line options. */ - auto alpha = [](const vecs_t& a, const vecs_t& b)->bool { + auto alpha = [](const vecinfo_t& a, const vecinfo_t& b)->bool { return QString::compare(a.desc, b.desc, Qt::CaseInsensitive) < 0; }; @@ -453,20 +465,19 @@ void Vecs::disp_vecs() const { const auto svp = sort_and_unify_vecs(); for (const auto& vec : svp) { - if (vec.vec->get_type() == ff_type_internal) { + if (vec.type == ff_type_internal) { continue; } printf(VEC_FMT, qPrintable(vec.name), qPrintable(vec.desc)); - const QVector* args = vec.vec->get_args(); - if (args) { - for (const auto& arg : *args) { - if (!(arg.argtype & ARGTYPE_HIDDEN)) - printf(" %-18.18s %s%-.50s %s\n", - arg.argstring, - (arg.argtype & ARGTYPE_TYPEMASK) == - ARGTYPE_BOOL ? "(0/1) " : "", - arg.helpstring, - (arg.argtype & ARGTYPE_REQUIRED) ? "(required)" : ""); + const QVector args = vec.arginfo; + for (const auto& arg : args) { + if (!(arg.argtype & ARGTYPE_HIDDEN)) { + printf(" %-18.18s %s%-.50s %s\n", + qPrintable(arg.argstring), + (arg.argtype & ARGTYPE_TYPEMASK) == + ARGTYPE_BOOL ? "(0/1) " : "", + qPrintable(arg.helpstring), + (arg.argtype & ARGTYPE_REQUIRED) ? "(required)" : ""); } } } @@ -481,16 +492,15 @@ void Vecs::disp_vec(const QString& vecname) const } printf(VEC_FMT, qPrintable(vec.name), qPrintable(vec.desc)); - const QVector* args = vec.vec->get_args(); - if (args) { - for (const auto& arg : *args) { - if (!(arg.argtype & ARGTYPE_HIDDEN)) - printf(" %-18.18s %s%-.50s %s\n", - arg.argstring, - (arg.argtype & ARGTYPE_TYPEMASK) == - ARGTYPE_BOOL ? "(0/1) " : "", - arg.helpstring, - (arg.argtype & ARGTYPE_REQUIRED) ? "(required)" : ""); + const QVector args = vec.arginfo; + for (const auto& arg : args) { + if (!(arg.argtype & ARGTYPE_HIDDEN)) { + printf(" %-18.18s %s%-.50s %s\n", + qPrintable(arg.argstring), + (arg.argtype & ARGTYPE_TYPEMASK) == + ARGTYPE_BOOL ? "(0/1) " : "", + qPrintable(arg.helpstring), + (arg.argtype & ARGTYPE_REQUIRED) ? "(required)" : ""); } } } @@ -521,9 +531,9 @@ void Vecs::disp_v1(ff_type t) printf("%s\t", tstring); } -void Vecs::disp_v2(const Format* v) +void Vecs::disp_v2(const vecinfo_t& v) { - for (auto& i : v->get_cap()) { + for (auto& i : v.cap) { putchar((i & ff_cap_read) ? 'r' : '-'); putchar((i & ff_cap_write) ? 'w' : '-'); } @@ -548,35 +558,33 @@ const char* Vecs::name_option(uint32_t type) return at[0]; } -void Vecs::disp_help_url(const vecs_t& vec, const arglist_t* arg) +void Vecs::disp_help_url(const vecinfo_t& vec, const QString& argstring) { printf("\t" WEB_DOC_DIR "/fmt_%s.html", CSTR(vec.name)); - if (arg) { - printf("#fmt_%s_o_%s", CSTR(vec.name), arg->argstring); + if (!argstring.isEmpty()) { + printf("#fmt_%s_o_%s", CSTR(vec.name), CSTR(argstring)); } printf("\n"); } -void Vecs::disp_v3(const Vecs::vecs_t& vec) +void Vecs::disp_v3(const vecinfo_t& vec) { disp_help_url(vec, nullptr); - const QVector* args = vec.vec->get_args(); - if (args) { - for (const auto& arg : *args) { - if (!(arg.argtype & ARGTYPE_HIDDEN)) { - printf("option\t%s\t%s\t%s\t%s\t%s\t%s\t%s", - CSTR(vec.name), - arg.argstring, - arg.helpstring, - name_option(arg.argtype), - arg.defaultvalue ? arg.defaultvalue : "", - arg.minvalue ? arg.minvalue : "", - arg.maxvalue ? arg.maxvalue : ""); - } - disp_help_url(vec, &arg); - printf("\n"); + const QVector args = vec.arginfo; + for (const auto& arg : args) { + if (!(arg.argtype & ARGTYPE_HIDDEN)) { + printf("option\t%s\t%s\t%s\t%s\t%s\t%s\t%s", + CSTR(vec.name), + CSTR(arg.argstring), + CSTR(arg.helpstring), + name_option(arg.argtype), + arg.defaultvalue.isEmpty() ? "" : CSTR(arg.defaultvalue), + arg.minvalue.isEmpty() ? "" : CSTR(arg.minvalue), + arg.maxvalue.isEmpty() ? "" : CSTR(arg.maxvalue)); } + disp_help_url(vec, arg.argstring); + printf("\n"); } } @@ -598,14 +606,14 @@ void Vecs::disp_formats(int version) const * Version 0 skips internal types. */ if (version > 0) { - disp_v1(vec.vec->get_type()); + disp_v1(vec.type); } else { - if (vec.vec->get_type() == ff_type_internal) { + if (vec.type == ff_type_internal) { continue; } } if (version >= 2) { - disp_v2(vec.vec); + disp_v2(vec); } printf("%s\t%s\t%s%s%s\n", CSTR(vec.name), !vec.extensions.isEmpty() ? CSTR(vec.extensions) : "", diff --git a/vecs.h b/vecs.h index 5849cfcb4..2d2870a6a 100644 --- a/vecs.h +++ b/vecs.h @@ -206,6 +206,35 @@ private: QString parent; }; + struct arginfo_t { + arginfo_t() = default; + explicit arginfo_t(const arglist_t& arg) : + argstring(arg.argstring), + helpstring(arg.helpstring), + defaultvalue(arg.defaultvalue), + argtype(arg.argtype), + minvalue(arg.minvalue), + maxvalue(arg.maxvalue) + {} + + QString argstring; + QString helpstring; + QString defaultvalue; + uint32_t argtype{ARGTYPE_UNKNOWN}; + QString minvalue; + QString maxvalue; + }; + + struct vecinfo_t { + QString name; + QString desc; + QString extensions; + QString parent; + ff_type type{ff_type_file}; + QVector cap; + QVector arginfo; + }; + public: void init_vecs(); void exit_vecs(); @@ -223,11 +252,11 @@ public: private: static int is_integer(const char* c); - QVector sort_and_unify_vecs() const; + QVector sort_and_unify_vecs() const; static void disp_v1(ff_type t); - static void disp_v2(const Format* v); - static void disp_help_url(const vecs_t& vec, const arglist_t* arg); - static void disp_v3(const vecs_t& vec); + static void disp_v2(const vecinfo_t& v); + static void disp_help_url(const vecinfo_t& vec, const QString& argstring); + static void disp_v3(const vecinfo_t& vec); static bool validate_vec(const vecs_t& vec); private: -- 2.30.2